Skip to content

[AWS] Add LB Type in the infrastructure cluster object via install-config yaml#6074

Closed
miheer wants to merge 3 commits intoopenshift:masterfrom
miheer:nlb-field
Closed

[AWS] Add LB Type in the infrastructure cluster object via install-config yaml#6074
miheer wants to merge 3 commits intoopenshift:masterfrom
miheer:nlb-field

Conversation

@miheer
Copy link
Contributor

@miheer miheer commented Jul 4, 2022

In the ingress config cluster object we need to add api for AwsLBType which will store and persist the information of the desired LB type mentioned by the installer config API so that the ingress operator will refer the LB type from this object whenever someone accidentally deletes the ingress controller.

Currently, the default LB type i.e CLB/ELB gets set if you delete the ingress controller object even when the desired the LB type for default ingress controller
was set to NLB as ingress operator does not have a way to check the LB type as we don't store it.
This commit adds LB Type for AWS mentioned in the install-config.yaml in the infrastructure cluster object.

data/data/install.openshift.io_installconfigs.yaml A generated new field called lbType will be available to add load balancer type in the install-config.yaml for AWS
pkg/asset/manifests/ingress.go The ingress cluster object will be patched with this lbType provided by user so that we have the information related to the lbType persistent which can be referred by ingress operator when the default ingress controller is deleted.
pkg/asset/manifests/ingress_test.go Unit test to test setting of lbType in the ingress cluster object's spec and status as per the value specified in the install-config.yaml
pkg/explain/printer_test.go A new field to test called lbType which will be available to add the load balancer type in the install-config.yaml for AWS
pkg/types/aws/platform.go A new field called lbType will be available to add the load balancer type in the install-config.yaml for AWS

Enhacement proposal - openshift/enhancements#1148
OpenShift API - openshift/api#1209

Jira ticket - https://issues.redhat.com/browse/NE-942

@miheer miheer changed the title Add LB Type in the infrastructure cluster object. WIP: Add LB Type in the infrastructure cluster object. Jul 4, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 4, 2022
@openshift-ci openshift-ci bot requested review from jstuever and rna-afk July 4, 2022 08:17
@miheer miheer changed the title WIP: Add LB Type in the infrastructure cluster object. WIP: Add LB Type in the infrastructure cluster object via install-config yaml. Jul 4, 2022
@miheer miheer force-pushed the nlb-field branch 2 times, most recently from 4ab0b8a to bd223a6 Compare July 5, 2022 03:31
@miheer miheer changed the title WIP: Add LB Type in the infrastructure cluster object via install-config yaml. Add LB Type in the infrastructure cluster object via install-config yaml. Jul 7, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2022
@miheer miheer force-pushed the nlb-field branch 3 times, most recently from 16644a8 to 850c399 Compare July 7, 2022 05:58
@miheer
Copy link
Contributor Author

miheer commented Jul 7, 2022

/retest

@patrickdillon
Copy link
Contributor

Please separate vendoring into a separate commit to make reviewing easier.

It would also be good to separate the pr into logical, smaller commits per the contributing guidelines.

Are there any reference links for this work such as an enhancement or jira cards?

@miheer
Copy link
Contributor Author

miheer commented Jul 11, 2022

@patrickdillon thanks! I have done the changes you asked for. Please do let me know if anything is needed.

I have added the commit message in detail as to which file does what.

Also reference to enhancement proposal, API and jira ticket have been added in the commit message.

I am adding them here as well -

openshift/enhancements#1148

openshift/api#1209

openshift/cluster-ingress-operator#798

https://issues.redhat.com/browse/NE-942

@miheer
Copy link
Contributor Author

miheer commented Jul 12, 2022

/retest-required

@sadasu
Copy link
Contributor

sadasu commented Jul 12, 2022

/hold
Enhacement proposal - openshift/enhancements#1148
OpenShift API - openshift/api#1209
need to merge first.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2022
@miheer
Copy link
Contributor Author

miheer commented Jul 17, 2022

/retest

1 similar comment
@miheer
Copy link
Contributor Author

miheer commented Jul 17, 2022

/retest

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 3, 2022
@miheer miheer changed the title Add LB Type in the infrastructure cluster object via install-config yaml. WIP: Add LB Type in the infrastructure cluster object via install-config yaml. Aug 3, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 3, 2022
@candita
Copy link

candita commented Oct 10, 2022

openshift/release#32996 should make unit and verify-codegen tests optional.
/retest-required

@candita
Copy link

candita commented Oct 10, 2022

/refresh

@candita
Copy link

candita commented Oct 10, 2022

/test okd-scos-verify-codegen

@candita
Copy link

candita commented Oct 10, 2022

/hold cancel
/retest-required

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 10, 2022
@LorbusChris
Copy link
Contributor

/skip

@LorbusChris
Copy link
Contributor

/retest-required
/shrug

@openshift-ci openshift-ci bot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Oct 10, 2022
@LorbusChris
Copy link
Contributor

/unshrug

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 10, 2022

@LorbusChris: ¯\_(ツ)_/¯

Details

In response to this:

/unshrug

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot removed the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Oct 10, 2022
@LorbusChris
Copy link
Contributor

/test okd-scos-images

@LorbusChris
Copy link
Contributor

/retest images

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 10, 2022

@LorbusChris: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test aro-unit
  • /test e2e-agent-compact
  • /test e2e-aws-ovn
  • /test e2e-aws-ovn-upi
  • /test e2e-azure-ovn
  • /test e2e-azure-ovn-upi
  • /test e2e-gcp-ovn
  • /test e2e-gcp-ovn-upi
  • /test e2e-metal-ipi-ovn-ipv6
  • /test e2e-vsphere-ovn
  • /test e2e-vsphere-upi
  • /test gofmt
  • /test golint
  • /test govet
  • /test images
  • /test okd-images
  • /test okd-scos-images
  • /test okd-unit
  • /test okd-verify-codegen
  • /test openstack-manifests
  • /test shellcheck
  • /test tf-lint
  • /test unit
  • /test verify-codegen
  • /test verify-vendor
  • /test yaml-lint

The following commands are available to trigger optional jobs:

  • /test e2e-agent-mce
  • /test e2e-alibaba
  • /test e2e-aws-disruptive
  • /test e2e-aws-ovn-fips
  • /test e2e-aws-ovn-imdsv2
  • /test e2e-aws-ovn-proxy
  • /test e2e-aws-ovn-shared-vpc
  • /test e2e-aws-ovn-single-node
  • /test e2e-aws-upgrade
  • /test e2e-aws-upi-proxy
  • /test e2e-aws-workers-rhel8
  • /test e2e-azure-ovn-resourcegroup
  • /test e2e-azure-ovn-shared-vpc
  • /test e2e-azurestack
  • /test e2e-azurestack-upi
  • /test e2e-crc
  • /test e2e-gcp-ovn-shared-vpc
  • /test e2e-gcp-upgrade
  • /test e2e-gcp-upi-xpn
  • /test e2e-ibmcloud-ovn
  • /test e2e-libvirt
  • /test e2e-metal-assisted
  • /test e2e-metal-ipi
  • /test e2e-metal-ipi-ovn-dualstack
  • /test e2e-metal-ipi-swapped-hosts
  • /test e2e-metal-ipi-virtualmedia
  • /test e2e-metal-single-node-live-iso
  • /test e2e-nutanix
  • /test e2e-openstack
  • /test e2e-openstack-kuryr
  • /test e2e-openstack-parallel
  • /test e2e-openstack-proxy
  • /test e2e-openstack-upi
  • /test e2e-ovirt
  • /test e2e-vsphere-zones
  • /test okd-e2e-aws-ovn
  • /test okd-e2e-aws-upgrade
  • /test okd-e2e-gcp
  • /test okd-e2e-gcp-ovn-upgrade
  • /test okd-e2e-vsphere
  • /test okd-scos-e2e-aws-ovn
  • /test okd-scos-e2e-aws-upgrade
  • /test okd-scos-e2e-gcp
  • /test okd-scos-e2e-gcp-ovn-upgrade
  • /test okd-scos-e2e-vsphere
  • /test okd-scos-unit
  • /test okd-scos-verify-codegen
  • /test tf-fmt

Use /test all to run the following jobs that were automatically triggered:

  • pull-ci-openshift-installer-master-aro-unit
  • pull-ci-openshift-installer-master-e2e-agent-compact
  • pull-ci-openshift-installer-master-e2e-agent-mce
  • pull-ci-openshift-installer-master-e2e-aws-ovn
  • pull-ci-openshift-installer-master-e2e-aws-ovn-fips
  • pull-ci-openshift-installer-master-e2e-aws-ovn-imdsv2
  • pull-ci-openshift-installer-master-e2e-aws-ovn-proxy
  • pull-ci-openshift-installer-master-e2e-aws-ovn-shared-vpc
  • pull-ci-openshift-installer-master-e2e-aws-ovn-single-node
  • pull-ci-openshift-installer-master-e2e-azure-ovn
  • pull-ci-openshift-installer-master-e2e-azure-ovn-resourcegroup
  • pull-ci-openshift-installer-master-e2e-azure-ovn-shared-vpc
  • pull-ci-openshift-installer-master-e2e-azurestack
  • pull-ci-openshift-installer-master-e2e-gcp-ovn
  • pull-ci-openshift-installer-master-e2e-gcp-ovn-shared-vpc
  • pull-ci-openshift-installer-master-e2e-ibmcloud-ovn
  • pull-ci-openshift-installer-master-e2e-libvirt
  • pull-ci-openshift-installer-master-e2e-metal-assisted
  • pull-ci-openshift-installer-master-e2e-metal-ipi
  • pull-ci-openshift-installer-master-e2e-openstack
  • pull-ci-openshift-installer-master-e2e-ovirt
  • pull-ci-openshift-installer-master-e2e-vsphere-ovn
  • pull-ci-openshift-installer-master-gofmt
  • pull-ci-openshift-installer-master-golint
  • pull-ci-openshift-installer-master-govet
  • pull-ci-openshift-installer-master-images
  • pull-ci-openshift-installer-master-okd-e2e-aws-ovn
  • pull-ci-openshift-installer-master-okd-e2e-aws-upgrade
  • pull-ci-openshift-installer-master-okd-images
  • pull-ci-openshift-installer-master-okd-scos-e2e-aws-ovn
  • pull-ci-openshift-installer-master-okd-scos-e2e-aws-upgrade
  • pull-ci-openshift-installer-master-okd-scos-images
  • pull-ci-openshift-installer-master-okd-scos-unit
  • pull-ci-openshift-installer-master-okd-scos-verify-codegen
  • pull-ci-openshift-installer-master-okd-unit
  • pull-ci-openshift-installer-master-okd-verify-codegen
  • pull-ci-openshift-installer-master-shellcheck
  • pull-ci-openshift-installer-master-tf-fmt
  • pull-ci-openshift-installer-master-tf-lint
  • pull-ci-openshift-installer-master-unit
  • pull-ci-openshift-installer-master-verify-codegen
  • pull-ci-openshift-installer-master-verify-vendor
  • pull-ci-openshift-installer-master-yaml-lint
Details

In response to this:

/retest images

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@LorbusChris
Copy link
Contributor

/test images

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 46e95b3 and 2 for PR HEAD b81fae1 in total

@LorbusChris
Copy link
Contributor

@miheer this PR needs a rebase

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD fef1d0f and 1 for PR HEAD b81fae1 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD edd32e0 and 0 for PR HEAD b81fae1 in total

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2022
@openshift-merge-robot
Copy link
Contributor

@miheer: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 11, 2022

@miheer: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-single-node a3e069852169340c4cbf4c8cb7e975925f3a34a0 link false /test e2e-aws-single-node
ci/prow/e2e-aws-imdsv2 a3e069852169340c4cbf4c8cb7e975925f3a34a0 link false /test e2e-aws-imdsv2
ci/prow/e2e-aws-shared-vpc a3e069852169340c4cbf4c8cb7e975925f3a34a0 link false /test e2e-aws-shared-vpc
ci/prow/e2e-ibmcloud a3e069852169340c4cbf4c8cb7e975925f3a34a0 link false /test e2e-ibmcloud
ci/prow/e2e-aws-fips a3e069852169340c4cbf4c8cb7e975925f3a34a0 link false /test e2e-aws-fips
ci/prow/okd-e2e-gcp-ovn-upgrade 73a4710ae77ba2d3305c3e63c954da6a31ef7494 link false /test okd-e2e-gcp-ovn-upgrade
ci/prow/e2e-aws-ovn-imdsv2 b81fae1 link false /test e2e-aws-ovn-imdsv2
ci/prow/e2e-ibmcloud-ovn b81fae1 link false /test e2e-ibmcloud-ovn
ci/prow/okd-scos-e2e-aws-upgrade b81fae1 link false /test okd-scos-e2e-aws-upgrade
ci/prow/e2e-vsphere-ovn b81fae1 link true /test e2e-vsphere-ovn
ci/prow/okd-scos-verify-codegen b81fae1 link false /test okd-scos-verify-codegen
ci/prow/okd-scos-unit b81fae1 link false /test okd-scos-unit
ci/prow/okd-e2e-aws-ovn b81fae1 link false /test okd-e2e-aws-ovn
ci/prow/images b81fae1 link true /test images
ci/prow/okd-e2e-aws-upgrade b81fae1 link false /test okd-e2e-aws-upgrade
ci/prow/e2e-aws-ovn-fips b81fae1 link false /test e2e-aws-ovn-fips
ci/prow/e2e-aws-ovn-single-node b81fae1 link false /test e2e-aws-ovn-single-node
ci/prow/e2e-libvirt b81fae1 link false /test e2e-libvirt
ci/prow/e2e-aws-ovn-proxy b81fae1 link false /test e2e-aws-ovn-proxy
ci/prow/e2e-openstack b81fae1 link false /test e2e-openstack
ci/prow/okd-scos-images b81fae1 link true /test okd-scos-images
ci/prow/okd-scos-e2e-aws-ovn b81fae1 link false /test okd-scos-e2e-aws-ovn
ci/prow/e2e-gcp-ovn-shared-vpc b81fae1 link false /test e2e-gcp-ovn-shared-vpc
ci/prow/e2e-azurestack b81fae1 link false /test e2e-azurestack

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@candita
Copy link

candita commented Oct 11, 2022

Before, the only merge conflict was:
CONFLICT (rename/rename): Rename terraform/providers/random/vendor/github.com/dustinkirkland/golang-petname/LICENSE->vendor/cloud.google.com/go/compute/LICENSE in HEAD. Rename vendor/cloud.google.com/go/LICENSE->vendor/cloud.google.com/go/compute/LICENSE in b81fae1

Now, there are also:
Auto-merging vendor/modules.txt
CONFLICT (content): Merge conflict in vendor/modules.txt
Auto-merging vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.go
CONFLICT (content): Merge conflict in vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.go
Auto-merging vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.go
CONFLICT (content): Merge conflict in vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.go
Auto-merging vendor/github.com/openshift/api/operator/v1/types_ingress.go
Auto-merging vendor/github.com/openshift/api/operator/v1/types_console.go
CONFLICT (content): Merge conflict in vendor/github.com/openshift/api/operator/v1/types_console.go
Auto-merging vendor/github.com/openshift/api/operator/v1/0000_70_dns-operator_00.crd.yaml
Auto-merging vendor/github.com/openshift/api/operator/v1/0000_70_console-operator.crd.yaml
CONFLICT (content): Merge conflict in vendor/github.com/openshift/api/operator/v1/0000_70_console-operator.crd.yaml
Auto-merging vendor/github.com/openshift/api/operator/v1/0000_50_ingress-operator_00-ingresscontroller.crd.yaml
Auto-merging vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.go
CONFLICT (content): Merge conflict in vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.go
Auto-merging vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.go
CONFLICT (modify/delete): vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_infrastructure.crd.yaml deleted in HEAD and modified in b81fae1. Version b81fae1 of vendor/github.com/openshift/api/config/v1/0000_10_config-operator_01_infrastructure.crd.yaml left in tree.

@gcs278
Copy link

gcs278 commented Oct 11, 2022

This PR has been open a while and now, there is no need to vendor anything because the current openshift-api version is: 20221004120407-c46852673d03.

I'm going to try to remove the vendoring commits, but not sure if I can modify this PR.

@gcs278
Copy link

gcs278 commented Oct 11, 2022

I have opened #6478 in lieu of this one so I could modify. I just removed the vendoring changes and rebased.

PTAL

@r4f4
Copy link
Contributor

r4f4 commented Oct 12, 2022

/close
in favor of #6478

@r4f4
Copy link
Contributor

r4f4 commented Oct 12, 2022

/close

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 12, 2022

@r4f4: Closed this PR.

Details

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot closed this Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants